-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add action widgets, open page widgets, and gauges on the Lock Screen #2830
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Looking good! I will manually test and review the PR this week |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2830 +/- ##
==========================================
- Coverage 27.54% 27.53% -0.02%
==========================================
Files 311 357 +46
Lines 31699 24572 -7127
==========================================
- Hits 8733 6767 -1966
+ Misses 22966 17805 -5161 ☔ View full report in Codecov by Sentry. |
btw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Just some general improvements that I could spot
Sources/Extensions/AppIntents/Widget/Details/WidgetDetailsAppIntent.swift
Outdated
Show resolved
Hide resolved
Sources/Extensions/AppIntents/Widget/Details/WidgetDetailsAppIntentTimelineProvider.swift
Outdated
Show resolved
Hide resolved
Sources/Extensions/AppIntents/Widget/Gauge/WidgetGaugeAppIntent.swift
Outdated
Show resolved
Hide resolved
Sources/Extensions/AppIntents/Widget/Gauge/WidgetGaugeAppIntent.swift
Outdated
Show resolved
Hide resolved
Sources/Extensions/AppIntents/Widget/Gauge/WidgetGaugeAppIntent.swift
Outdated
Show resolved
Hide resolved
Sources/Extensions/Widgets/Common/WidgetBasicContainerView.swift
Outdated
Show resolved
Hide resolved
Sources/Shared/Notifications/NotificationCommands/NotificationsCommandManager.swift
Outdated
Show resolved
Hide resolved
Sources/Shared/Notifications/NotificationCommands/NotificationsCommandManager.swift
Outdated
Show resolved
Hide resolved
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
You also need this code on line 81 in
|
Sources/Shared/Notifications/NotificationCommands/NotificationsCommandManager.swift
Outdated
Show resolved
Hide resolved
I haven’t managed to test the notification commands at all. I can’t seem to get my own firebase app connected to APNs properly. |
Sources/PushServer/SharedPush/Sources/NotificationParserLegacy.swift
Outdated
Show resolved
Hide resolved
@@ -19,6 +20,9 @@ public class NotificationCommandManager { | |||
public init() { | |||
register(command: "request_location_update", handler: HandlerLocationUpdate()) | |||
register(command: "clear_notification", handler: HandlerClearNotification()) | |||
if #available(watchOS 9, *) { | |||
register(command: "reload_widgets", handler: HandlerReloadWidgets()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the created enum, this is a good example of where things could go wrong, now you are registering "reload_widgets" but on that enum is "update_widgets"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, let's remove the #available there and move this inside the #if os(iOS) on line 26, since it doesnt make sense to have it for watch anyways
Localization didn't work on my side, do you see it working fine when you edit the widget? |
I just manually tested it, and after the changes related to the enum above, all works well, we just need to add a note in the documentation telling the user that they can't keep reloading the widgets in a short period of time, since iOS will refuse to do it that often. Btw, after these changes above we are good to merge, could you just update the docs? repo: |
Localization was working fine for me until I changed my locale. The localized string resources don’t seem to have a fallback for when the string isn’t in the current locale unless you explicitly set it, so I just added defaults for English. |
….swift Co-authored-by: Bruno Pantaleão Gonçalves <[email protected]>
Sources/Shared/Notifications/NotificationCommands/NotificationsCommandManager.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Anything else you want to add? Otherwise I can merge it (Asking because you didnt hit the "ready to review" button) |
Oh, no I just forgot to hit the button |
Thank you for your contribution! Don't forget a small update in https://companion.home-assistant.io/docs/notifications/notification-commands repo: |
Sorry for commenting but just wondering if any documentation for these new widgets will be added? |
Summary
Allow action widget, open page widget, and watchOS style complications to be available on the iOS Lock Screen.
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes